Review: MegaMek PR #8946 "Buildings" (exeea/buildings -> main)

Reviewed at head 6629ddc (merge-base = current main b3891f1). 283 files, +24,116 / -1,102. Checked against the project coding style guide and PR checklist.
Every finding below was verified against the code, the CI logs, or a
local build/test run of the branch. Each item: what was checked, what was found, what to do.

Verdict: NOT MERGEABLE as-is. Five blockers (B1-B5), then a long tail of should-fix items. The architecture is
sound in several places (shared client/server rule, XStream record converters, validated CFR paths), so the
work is salvageable; the problems are wire compatibility, red CI, failing own tests, scope, and style volume.

A. Blockers

B. Server-side gate

C. Monolithic-class gate

D. Deep dive: deployment facing (requested)

Files: DeploymentDisplay.processTurn / determineDeploymentPosition, AllowedDeploymentHelper.hasFacingDependentFootprint, AbstractBuildingEntity.isDeploymentPositionAndFacingValid, DeploymentProcessor.receiveDeployment.

E. Style and hygiene (0, 2)

F. Diagnostic logging

G. Performance

H. Tests

I. Automated review triage

Verified each against the code. Accept the valid ones, decline the rest with the stated reason.

J. Spotless warning before you format

spotlessApply strips import java.lang.System; as redundant. BoardView and MoveStep both carry that import
because they wildcard-import megamek.common.units.*, which contains a System enum, and both call
System.currentTimeMillis(). After formatting they will not compile. Change those calls to
java.lang.System.currentTimeMillis() first, then format. (Known trap on this project.)

K. Public API surface

L. PR body and process

• Body is one sentence. The repo template (What this changes / Testing / What is not proven yet + checklist) is unused; no issue link.
• No AI Assisted Development label despite AI-assisted authorship.
• No statement of what was played in game. Given the size, the body must say which parts were exercised live (deploy a multi-hex building, open a door, move a mobile structure, sink one) and which were unit-tested only.

M. What is done well (keep)

• Shared client/server deployment rule; non-mutating validity check.
• XStream converters registered for every Serializable record; Java and XStream round-trip tests; legacy .ser fixture.
• Lazy init of wallSegmentState / buildingRuntimeState; Game.readObject migration for the elevator map.
• Bay.canLoad(Entity) preserved for MekHQ.
• CFR-supplied MovePath recompiled and validated server-side before use.
• Bot handling placed in BotClient, so Princess and CASPAR both get it.
• Board.collapseBuilding uses setHex.

Follow-up: deployment orientation and duplicated logic

Two items I was asked to look at specifically after the main review.

Deployment orientation (section D of the review, condensed)

• Clicking a multi-hex building's own origin hex while turning it rotates the building to a junk facing. The new same-hex guard in processTurn sits after the multi-hex branch, so it never protects buildings; turnBuildingToward then computes position.direction(position). Reachable via hexMoused:936 with shift-click or Turn mode. The PR's own test shows it: clickingTheOriginDoesNotResetFacing expects 4 and gets 5.
• Facings are offered at elevation 0 (determineDeploymentPosition:876, FACING_ELEVATION) but validated at the chosen elevation at :907 and in DeploymentProcessor:120. A sub-surface or raised building can be offered a facing it is then refused, with no valid option reachable.
• hasFacingDependentFootprint is still footprint > 1, but hexside walls and mobile portals now make single-hex validity facing-dependent inside isDeploymentPositionAndFacingValid. Single-hex walls take the plain-unit path and get no facing chooser.
• The rotation maths itself agrees between client and server; DeploymentProcessorTest stamps the correct footprint for all six facings.

Duplicated logic

Checked and not duplicated: the three collapse handlers compose rather than repeat; BLKStructureFile delegates to BuildingDesignCodec; the rollMovementInBuilding overload delegates; MML BuildingUtil calls BuildingConstruction and TestBuilding rather than re-deriving the rules.

Overlap with an in-progress branch

Implement-Building-Editor-Hex-Layout (pushed, not yet a PR) adds a hex-map view to the in-game damage editor. It touches four of the same files as this PR: AbstractBuildingEntity, UnitDamageControls, UnitDamagePanelBuilder, and the client messages.properties. Whichever lands second will conflict there.

It also names building hexes differently. That branch adds BuildingHexNames.hexName driven by AbstractBuildingEntity.getHexIndex(location); #2328 adds BuildingUtil.locationLabel (sheet-grid label plus level) and absoluteHexLabel (q,r). The same hex would carry one name in MegaMek's damage editor and another on the MegaMekLab sheet. That naming belongs in one helper in megamek.common that both apps call. Happy to coordinate on which one wins.

Recommendation: resolving the duplication

For each area: the one implementation that survives, what the others become, and the functionality each side needs kept.

1. Hex rotation: one method on the record

• Keep: a new CubeCoords.rotate(int turns) (one clockwise step is (-r, -s, -q), floorMod on turns). The signed-zero fix in the CubeCoords constructor stays; rotation is what produces -0.0.
• Remove: AbstractBuildingEntity.rotateCoordByFacing (delegate, then delete), MobileStructureLinkage.rotate; the lambda in MML BuildingUtil.rotate becomes hex -> hex.rotate(1).
• Test: rotate(6) is identity, and rotate(1) of each unit vector matches the neighbour in the next facing. That pins the "facing 0 is up, clockwise" convention the three copies currently assume silently.

2. Facing predicates: two concepts, one home (AllowedDeploymentHelper)

• isBuildingWithFacingRules(entity) = any AbstractBuildingEntity. Drives findAllowedFacings, the Turn button label, and processTurn routing to turnBuildingToward / turnBuildingToValidFacing. Those already toast and log, so the single-hex toast branch in processTurn and the buildingCannotRotate message go away.
• hasFacingDependentFootprint(entity) = multi-hex only. Drives only isFacingClickOnPlacedBuilding: a single-hex building keeps click-to-move on adjacent hexes, a multi-hex one keeps click-to-turn.
• Delete isFacingDependentDeployment; make getValidFacingsAt delegate to the deployment-valid loop and deprecate it.
• Gain: single-hex walls and portals get the facing chooser with invalid facings greyed. Symmetric single-hex buildings see all six enabled, which is correct.

3. Deployment validity: one rule that says why

• Turn isDeploymentPositionAndFacingValid into deploymentRejection(position, facing, elevation, boardId) returning a small enum (NONE, OFF_BOARD, UNEVEN, OVERLAP, OCCUPIED, OUTSIDE_ZONE, NO_SURFACE_PART, PORTAL_UNMATCHED, WALL_OVERLAP). The boolean form becomes == NONE.
• Three gates, same rule: findAllowedFacings(finalElevation) builds the menu after the elevation is chosen; checkNags is the client's last gate; DeploymentProcessor is the server's. Delete the check at determineDeploymentPosition:907.
• Gain: the client toast and the server log both name the reason. The elevation mismatch disappears because only one elevation is in play.

4. Elevation: design offset versus runtime absolute

These are two different quantities, so the fix is naming and routing, not merging.

• BuildingConstruction.baseLevel is an authoring value (lowest floor relative to the building's own ground): rename designBaseLevel. MobileStructure.getStructureBaseElevation is the same kind of value: rename designBaseOffset and keep the two together.
• BuildingElevation is the only runtime facade: base, roof, floor, contains, at. Give IBuilding a default getDeckLevel(coords) so the two bridge cases (terrain bridge, entity bridge) stop being an instanceof ladder inside base.
• MobileStructure.getBaseElevation(coords) stays as the mobile implementation but is reached only through the facade. Route the 25 direct callers through BuildingElevation.base so the sinking, tunnel and VTOL cases cannot be bypassed.
• Test: one BuildingElevationTest covering surface, underground, terrain bridge, entity bridge, tracked mobile on a tunnel, sinking naval, VTOL. Today those cases are split across three implementations with no single test.

5. Hex naming: one identity, two views

The two schemes serve different audiences, so keep both views but give them one identity.

• Identity: the hex's number in the unit file's declaration order, 1-based. Implement-Building-Editor-Hex-Layout already has getHexIndex(location); promote it to AbstractBuildingEntity with a getHexNumber(CubeCoords). It is stable across rotation because BuildingUtil.rotate preserves list order, and across re-save because the file is that list.
• Shared helper: move BuildingHexNames from the client package to megamek.common.units as BuildingHexLabels, with hexName(building, index) giving "Hex 3" or "Hex 3 (0304)" once placed, and locationName(building, location) giving "Hex 3, Level 2". Its message keys move to the common bundle so MegaMekLab can read them.
• Sheet view (Add subcapital cannon ammo weights #2328): keep SheetGrid for drawing the map, but print the hex number inside every drawn hex and use locationName in the inventory location column. The "0104" grid label becomes a drawing coordinate only. Drop absoluteHexLabel from anything a person reads.
• Gain: "Hex 3, Level 2" in the in-game damage editor is the same hex the printed sheet calls "Hex 3, Level 2".

6. Order of landing

Land Implement-Building-Editor-Hex-Layout first (738 lines, already pushed, owns the identity in item 5). This PR has to be split anyway; its shared-model slice rebases on top and adopts getHexIndex and BuildingHexLabels. Items 1 to 4 belong in that same slice, since every later slice depends on them.

Once this is ready for re-review, @ me on discord so I don't miss it